Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libvncserver: handle EINTR #483

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

libvncserver: handle EINTR #483

wants to merge 1 commit into from

Conversation

piec
Copy link

@piec piec commented Sep 27, 2021

Hi,

I'm using libvncserver in a program that uses many signals, I have no control over that.
I noticed that this select sometimes gets an EINTR.

The proposed fix is not pretty because it uses a goto but it's also simple & readable.
Please tell me if you'd prefer a do {} while loop. The disadvantage is that the break after would have to changed.

Thanks for this awesome lib
Pierre

PS: I suspect other syscalls could have issues too

n = select(nfds + 1, &rfds, &wfds, &efds, &tv);

if (n < 0) {
rfbLogPerror("ReadExact: select");
if (errno == EINTR) {
goto retry;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why wouldn't a simple continue work as in the other cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piec the goto itself is not an issue, the problem is if the client disconnect.
In that case the FD will become invalid, but your retry will cut the check done some line before, and it will never finish till it'll probably crash.
So please try with the continue, I cannot reproduce the problem to test.

@bk138 bk138 self-assigned this Apr 10, 2022
@piec
Copy link
Author

piec commented Apr 11, 2022

I don't remember if I've tried it, I'll have to check again

@bk138
Copy link
Member

bk138 commented Apr 11, 2022

I don't remember if I've tried it, I'll have to check again

That'd be nice - thank's in advance!

@bk138
Copy link
Member

bk138 commented Apr 28, 2022

Needs a check if continue does the job as well.

@piec
Copy link
Author

piec commented Apr 28, 2022

I'll get some time to look at it at the beginning of next week

@bk138 bk138 removed their assignment Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants